Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wait for Account proofs blocks if they are not available yet #3159

Merged
merged 2 commits into from
Nov 26, 2024

Conversation

nibhar
Copy link
Member

@nibhar nibhar commented Nov 26, 2024

When getting the trie in the RemoteDataStore previous to this PR the block which is referenced by the returned proof must already exist locally. That is by no means guaranteed and will result in discarding the proof.

This PR thus added a waiting window within which the RemoteDataStore will wait for the block to arrive. It will wait a specified amount of blockchain events which have added blocks.

Additionally whenever a proof would not verify another peer will be asked to proof an address, whereas before this PR the get_trie would simply fail at this point.

A button was added to the web client example to exercise the behaviour.

@nibhar nibhar requested review from sisou, jsdanielh and paberr November 26, 2024 20:52
@nibhar nibhar self-assigned this Nov 26, 2024
@paberr
Copy link
Member

paberr commented Nov 26, 2024

Added a comment, but generally looks good.

@jsdanielh jsdanielh force-pushed the nibhar/account-proofs branch 2 times, most recently from 4bea92c to b415376 Compare November 26, 2024 21:33
@nibhar nibhar force-pushed the nibhar/account-proofs branch 2 times, most recently from d613688 to 220a6b7 Compare November 26, 2024 22:04
Copy link
Member

@jsdanielh jsdanielh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jsdanielh
Copy link
Member

Signing commit

@jsdanielh jsdanielh force-pushed the nibhar/account-proofs branch from 220a6b7 to 5720cf2 Compare November 26, 2024 22:06
Wait for a set amount of events only

Also continue with the next peer if it fails for the first peer instead of returning from the function.
@jsdanielh jsdanielh force-pushed the nibhar/account-proofs branch from 5720cf2 to 3936e2f Compare November 26, 2024 22:18
hrxi added a commit that referenced this pull request Nov 26, 2024
Previously, the function would try to block the whole thread whenever
requesting something from other peers.

This wasn't noticed since the implementation is only used in light
clients that were **NOT** compiled for WASM.

I noticed this implementation when reviewing #3159, which would have
caused these thread blockings to continue for longer times.
hrxi added a commit that referenced this pull request Nov 26, 2024
Previously, the function would try to block the whole thread whenever
requesting something from other peers.

This wasn't noticed since the implementation is only used in light
clients that were **NOT** compiled for WASM.

I noticed this implementation when reviewing #3159, which would have
caused these thread blockings to continue for longer times.
@jsdanielh jsdanielh merged commit 3936e2f into albatross Nov 26, 2024
8 checks passed
@jsdanielh jsdanielh deleted the nibhar/account-proofs branch November 26, 2024 22:49
hrxi added a commit that referenced this pull request Nov 27, 2024
Previously, the function would try to block the whole thread whenever
requesting something from other peers.

This wasn't noticed since the implementation is only used in light
clients that were **NOT** compiled for WASM.

I noticed this implementation when reviewing #3159, which would have
caused these thread blockings to continue for longer times.
@sisou
Copy link
Member

sisou commented Dec 7, 2024

@nibhar turns out there was already this while loop in the client. Why wasn't it working, and can it be removed?

while let Some((notification, _)) = address_notifications.next().await {
{
loop {
let current_block_number =
consensus.blockchain.read().head().block_number();
if notification
.receipts
.iter()
.any(|(_, block_number)| block_number > &current_block_number)
{
log::debug!("Received transaction receipt(s) from the future, waiting for the blockchain head to be updated...");
let mut blockchain_events =
consensus.blockchain.read().notifier_as_stream();
let _ = blockchain_events.next().await;
} else {
break;
}
}
}

@sisou
Copy link
Member

sisou commented Dec 7, 2024

Ah, stupid me. That while loop is for the tx listener, not for account proofs 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants